Conversation
…down support and everything
|
Caution Review failedThe pull request is closed. WalkthroughAdds a multi-page onboarding flow (OnboardingModal, OnboardingFeatures) and multiple onboarding UI components (FakeLayout, FakeBlock, FakeChat, CommandReveal/FakeCommand and composables). Replaces TosModal usage with OnboardingModal in the modal registry and renderer. Introduces EmojiButton, updates QuickTips with multi-chord keybinding rendering and new responsive layout, makes streamdown details collapsible, and adds a Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
frontend/app/element/streamdown.tsx (2)
181-198: Consider adding ARIA attributes for better accessibility.The Collapsible button should include
aria-expandedto communicate its state to assistive technologies. Additionally, consider addingaria-controlsto link the button to the content region.Apply this diff to improve accessibility:
+ const contentId = `collapsible-${title.toString().replace(/\s+/g, '-').toLowerCase()}`; + return ( <div className="my-3"> <button className="flex items-center gap-2 cursor-pointer bg-transparent border-0 p-0 font-medium text-secondary hover:text-primary" onClick={() => setIsOpen(!isOpen)} + aria-expanded={isOpen} + aria-controls={contentId} > <span className="text-[0.65rem] text-primary transition-transform duration-200 inline-block w-3"> {isOpen ? "\u25BC" : "\u25B6"} {/* ▼ ▶ */} </span> <span>{title}</span> </button> - {isOpen && <div className="mt-2 ml-1 pl-3.5 border-l-2 border-border text-secondary">{children}</div>} + {isOpen && <div id={contentId} className="mt-2 ml-1 pl-3.5 border-l-2 border-border text-secondary">{children}</div>} </div> );
278-292: Consider defensive checks for robustness.The summary extraction logic assumes a specific node structure from the markdown parser (
c?.props?.node?.tagName). While this likely works consistently with the streamdown library, consider adding a type guard or more defensive extraction to handle edge cases where the parser output might differ.Optional: Add a helper function to safely extract the summary:
const extractSummary = (children: React.ReactNode) => { const childArray = Array.isArray(children) ? children : [children]; const summary = childArray.find((c) => c && typeof c === 'object' && 'props' in c && c.props?.node?.tagName === "summary" ); return summary?.props?.children || "Details"; };pkg/wcore/workspace.go (1)
222-222: Update comment to reflect OnboardingModal.The comment references "TOS modal dismissal" but the PR replaces the TOS modal with OnboardingModal. Update the comment for accuracy.
Apply this diff to update the comment:
- // No need to apply an initial layout for the initial launch, since the starter layout will get applied after TOS modal dismissal + // No need to apply an initial layout for the initial launch, since the starter layout will get applied after OnboardingModal dismissalfrontend/app/onboarding/onboarding-layout.tsx (3)
38-41: Avoid reusing a static CodeEditor blockId across instancesAll FakeBlocks share blockId="fake-block". This can collide editor overrides/state and cause cross‑talk. Use a per‑instance id.
Apply this diff:
- <CodeEditor blockId="fake-block" text={editorText} readonly={true} language="shell" /> + <CodeEditor blockId={localBlockId} text={editorText} readonly={true} language="shell" />Add inside FakeBlock (near the component start):
const localBlockId = useRef(`fake-block-${crypto?.randomUUID?.() ?? Math.random().toString(36).slice(2)}`).current;
64-75: Recompute measured rect on resize/relayoutblockRect is captured only once. Resizes or container layout changes will desync the overlay. Observe size/position changes.
You can attach a ResizeObserver to highlightedContainerRef and layoutRef to update setBlockRect on size changes. Example:
useLayoutEffect(() => { const elem = highlightedContainerRef.current; if (!elem) return; const ro = new ResizeObserver(() => { setBlockRect({ left: elem.offsetLeft, top: elem.offsetTop, width: elem.offsetWidth, height: elem.offsetHeight, }); }); ro.observe(elem); return () => ro.disconnect(); }, []);
150-161: Don’t intercept pointer events when collapsedThe animated FakeBlock can still capture clicks when not expanded. Disable pointer events in the collapsed state.
Apply this diff:
- <div className="absolute transition-all duration-200 ease-in-out" style={getAnimatedStyle()}> + <div + className={cn("absolute transition-all duration-200 ease-in-out", !isExpanded && "pointer-events-none")} + style={getAnimatedStyle()} + >frontend/app/element/quicktips.tsx (1)
289-297: Harden external links: add noreferrerFor target="_blank" links, include rel="noopener noreferrer" to prevent referrer leakage and strengthen security.
Apply this diff:
- <a - target="_blank" - href="https://discord.gg/XfvZ334gwU" - rel="noopener" + <a + target="_blank" + href="https://discord.gg/XfvZ334gwU" + rel="noopener noreferrer" @@ - <a - target="_blank" - href="https://docs.waveterm.dev/config" - rel="noopener" + <a + target="_blank" + href="https://docs.waveterm.dev/config" + rel="noopener noreferrer" @@ - <a - target="_blank" - href="https://docs.waveterm.dev/keybindings" - rel="noopener" + <a + target="_blank" + href="https://docs.waveterm.dev/keybindings" + rel="noopener noreferrer" @@ - <a - target="_blank" - href="https://docs.waveterm.dev" - rel="noopener" + <a + target="_blank" + href="https://docs.waveterm.dev" + rel="noopener noreferrer"Also applies to: 302-310, 315-323, 328-336
frontend/app/onboarding/onboarding-command.tsx (1)
24-48: Use useEffect for timers and stop cursor when typing completesTimers don’t need layout sync; also stop the cursor blinker after completion and avoid starting it when hidden.
Apply this diff:
- useLayoutEffect(() => { + useEffect(() => { let charIndex = 0; - const typeInterval = setInterval(() => { + const typeInterval = window.setInterval(() => { if (charIndex < command.length) { setDisplayedText(command.slice(0, charIndex + 1)); charIndex++; } else { - clearInterval(typeInterval); + window.clearInterval(typeInterval); setIsComplete(true); - setShowCursor(false); + setShowCursor(false); if (onComplete) { onComplete(); } + // stop cursor blink after completion + if (cursorIntervalId) { + window.clearInterval(cursorIntervalId); + } } - }, typeIntervalMs); + }, typeIntervalMs); - const cursorInterval = setInterval(() => { - setShowCursor((prev) => !prev); - }, 500); + const cursorIntervalId = showCursorProp + ? window.setInterval(() => { + setShowCursor((prev) => !prev); + }, 500) + : undefined; return () => { - clearInterval(typeInterval); - clearInterval(cursorInterval); + window.clearInterval(typeInterval); + if (cursorIntervalId) window.clearInterval(cursorIntervalId); }; - }, [command, typeIntervalMs, onComplete]); + }, [command, typeIntervalMs, onComplete, showCursorProp]);frontend/app/modals/modalsrenderer.tsx (1)
23-24: Use a fallback key for OnboardingModaldisplayName may be undefined; provide a stable fallback to avoid React key warnings.
Apply this diff:
- rtn.push(<OnboardingModal key={OnboardingModal.displayName} />); + rtn.push(<OnboardingModal key={OnboardingModal.displayName || "OnboardingModal"} />);frontend/app/onboarding/onboarding-features.tsx (1)
206-297: Consider simplifying command cycling.The command cycling logic works correctly but updates both
commandIndexandkeystate (Lines 232-233). SincecommandIndexchanges already force a re-render and the render function at Line 291 usescommandIndexdirectly, the separatekeystate update may be unnecessary.If you find the separate key tracking redundant, consider this simplification:
- const [key, setKey] = useState(0); const handleCommandComplete = () => { setTimeout(() => { setCommandIndex((prev) => (prev + 1) % commands.length); - setKey((prev) => prev + 1); }, 2500); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
docs/docs/keybindings.mdx(3 hunks)frontend/app/element/emojibutton.tsx(1 hunks)frontend/app/element/quicktips.tsx(1 hunks)frontend/app/element/streamdown.tsx(3 hunks)frontend/app/modals/modalregistry.tsx(1 hunks)frontend/app/modals/modalsrenderer.tsx(2 hunks)frontend/app/onboarding/fakechat.tsx(1 hunks)frontend/app/onboarding/onboarding-command.tsx(1 hunks)frontend/app/onboarding/onboarding-features.tsx(1 hunks)frontend/app/onboarding/onboarding-layout.tsx(1 hunks)frontend/app/onboarding/onboarding.tsx(4 hunks)frontend/app/store/keymodel.ts(4 hunks)frontend/app/workspace/widgets.tsx(1 hunks)frontend/tailwindsetup.css(1 hunks)frontend/types/gotypes.d.ts(3 hunks)pkg/telemetry/telemetrydata/telemetrydata.go(2 hunks)pkg/waveobj/metaconsts.go(1 hunks)pkg/waveobj/wtypemeta.go(1 hunks)pkg/wcore/workspace.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
frontend/app/modals/modalregistry.tsx (1)
frontend/app/onboarding/onboarding.tsx (1)
OnboardingModal(287-287)
frontend/app/onboarding/fakechat.tsx (1)
frontend/app/element/streamdown.tsx (1)
WaveStreamdown(208-327)
frontend/app/onboarding/onboarding-command.tsx (1)
frontend/app/onboarding/onboarding-layout.tsx (1)
FakeBlock(20-54)
frontend/app/onboarding/onboarding-layout.tsx (3)
frontend/app/element/magnify.tsx (1)
MagnifyIcon(12-18)frontend/app/view/codeeditor/codeeditor.tsx (1)
CodeEditor(115-171)frontend/app/element/streamdown.tsx (1)
WaveStreamdown(208-327)
frontend/app/modals/modalsrenderer.tsx (1)
frontend/app/onboarding/onboarding.tsx (1)
OnboardingModal(287-287)
frontend/app/element/quicktips.tsx (3)
frontend/util/util.ts (1)
cn(407-407)frontend/util/platformutil.ts (2)
PLATFORM(2-2)PlatformMacOS(1-1)frontend/app/element/magnify.tsx (1)
MagnifyIcon(12-18)
frontend/app/onboarding/onboarding.tsx (6)
frontend/app/workspace/workspace-layout-model.ts (1)
WorkspaceLayoutModel(298-298)frontend/app/store/wshclientapi.ts (1)
RpcApi(537-537)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/onboarding/onboarding-features.tsx (1)
OnboardingFeatures(299-351)frontend/app/store/keymodel.ts (2)
disableGlobalKeybindings(653-653)enableGlobalKeybindings(654-654)frontend/app/modals/modal.tsx (1)
FlexiModal(112-112)
frontend/app/onboarding/onboarding-features.tsx (8)
frontend/util/platformutil.ts (1)
isMacOS(8-10)frontend/app/store/wshclientapi.ts (1)
RpcApi(537-537)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/element/emojibutton.tsx (1)
EmojiButton(7-46)frontend/app/onboarding/fakechat.tsx (1)
FakeChat(237-269)frontend/app/element/magnify.tsx (1)
MagnifyIcon(12-18)frontend/app/onboarding/onboarding-layout.tsx (1)
FakeLayout(56-165)frontend/app/onboarding/onboarding-command.tsx (3)
EditBashrcCommand(113-130)ViewShortcutsCommand(86-103)ViewLogoCommand(105-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (21)
frontend/app/element/streamdown.tsx (2)
261-274: LGTM! Improved list typography.The spacing adjustments and
leading-snugon list items create better visual hierarchy and readability for markdown-rendered lists.
308-308: LGTM! Consistent vertical spacing.The
space-y-2utility provides uniform vertical rhythm between markdown elements, improving overall layout consistency.frontend/tailwindsetup.css (1)
101-110: LGTM!The
float-upanimation is well-defined and will provide a smooth floating effect for the EmojiButton component. The transform centering (-50%horizontal) combined with the vertical translation creates a nice upward fade-out effect.docs/docs/keybindings.mdx (1)
7-8: LGTM!The import path updates improve consistency by removing file extensions, aligning with the broader refactoring patterns in this PR.
frontend/app/workspace/widgets.tsx (1)
93-93: LGTM!Adding the
magnifiedflag to the tips widget aligns with the broader onboarding and magnification UI enhancements in this PR.frontend/app/store/keymodel.ts (1)
36-36: LGTM!The global keybindings disable/enable mechanism is well-implemented with:
- A simple module-level flag for efficient gating without React re-renders
- Clean early-return pattern in
appHandleKeyDown- Straightforward public API for enabling/disabling
This design is appropriate for low-level keyboard event filtering during onboarding flows.
Also applies to: 90-96, 373-375
frontend/types/gotypes.d.ts (1)
670-670: LGTM!The type additions for onboarding metadata and telemetry properties are well-defined and align with the corresponding backend changes in
pkg/telemetry/telemetrydata/telemetrydata.go.Also applies to: 937-937, 954-954
frontend/app/onboarding/fakechat.tsx (2)
239-240: Verify the initialchatIndexvalue.The
chatIndexis initialized to1, which means the component will display the second chat configuration (chatConfigs[1]) on initial render, skippingchatConfigs[0]. Is this intentional, or should it start at0?If starting at index 1 is not intentional, apply this diff:
- const [chatIndex, setChatIndex] = useState(1); + const [chatIndex, setChatIndex] = useState(0);
124-173: LGTM!The animation state machine and timing logic are well-implemented:
- Proper phase transitions (thinking → tool → streaming)
- All timeouts and intervals are correctly cleaned up in the return function
- Auto-scrolling behavior enhances the demo experience
Also applies to: 242-250
frontend/app/element/emojibutton.tsx (1)
7-46: LGTM!The
EmojiButtoncomponent is well-implemented with:
- Proper use of
useLayoutEffectanduseRefto detect click state transitions- Coordinated timing between the floating label timeout (600ms) and CSS animation duration
- Correct positioning and transform for the floating effect
The component integrates nicely with the
float-upanimation defined intailwindsetup.css.pkg/telemetry/telemetrydata/telemetrydata.go (1)
34-36: LGTM!The telemetry additions for onboarding are well-defined:
- Event names follow existing naming conventions (
onboarding:start,onboarding:skip,onboarding:fire)OnboardingFeaturefield has proper JSON tags and TypeScript type constraints- Changes align with the corresponding frontend type definitions in
frontend/types/gotypes.d.tsAlso applies to: 92-92
frontend/app/modals/modalregistry.tsx (1)
9-14: LGTM: registry now points to OnboardingModalMapping and fallback key logic look good.
pkg/waveobj/wtypemeta.go (1)
139-139: LGTM!The new
OnboardingGithubStarfield follows the existing metadata pattern correctly with proper json tagging and theomitemptydirective. The alignment with the corresponding constant inmetaconsts.gois consistent.pkg/waveobj/metaconsts.go (1)
135-135: Verify manual edit to generated code.The file header at Line 4 states
// Generated Code. DO NOT EDIT., but this constant appears to be manually added. If this file is indeed auto-generated, the new constant should be added through the generation process rather than manually.Confirm whether this file is generated and, if so, ensure the constant is added via the proper code generation workflow.
frontend/app/onboarding/onboarding.tsx (3)
42-46: LGTM!The conditional logic correctly sets AI panel visibility when telemetry is enabled and navigates to the appropriate next page based on the telemetry state. The synchronous call to
setAIPanelVisible(true)before navigation ensures the UI state is ready for the features page.
149-166: LGTM!The GitHub star prompt handlers correctly record the user's choice using
SetMetaCommandwith the appropriate metadata key and navigate to the features page. The async/await pattern is properly used.
252-257: LGTM!Disabling global keybindings while the modal is open prevents unintended keyboard shortcuts from interfering with the onboarding flow. The cleanup function properly re-enables them when the modal unmounts.
frontend/app/onboarding/onboarding-features.tsx (4)
18-61: LGTM!The
OnboardingFootercomponent correctly implements navigation controls with conditional rendering for prev/skip buttons and progress indication. The layout with absolute positioning for prev/skip on the sides and centered next/finish button is clear and user-friendly.
63-139: LGTM!The
WaveAIPagecorrectly adapts keyboard shortcuts for macOS vs other platforms and records telemetry for user engagement. The layout effectively combines instructional content with a live preview of the AI chat feature.
141-204: LGTM!The
MagnifyBlocksPagefollows the same successful pattern as the WaveAI page with platform-aware shortcuts and telemetry tracking. The visual demonstration withFakeLayouteffectively shows the magnify feature in action.
299-351: LGTM!The
OnboardingFeaturesorchestrator correctly manages the multi-page flow with appropriate telemetry events for tracking user progression through the onboarding. The linear navigation with skip option provides a good user experience.
| const timeouts: NodeJS.Timeout[] = []; | ||
|
|
||
| const addTimeout = (callback: () => void, delay: number) => { | ||
| const id = setTimeout(callback, delay); | ||
| timeouts.push(id); | ||
| }; |
There was a problem hiding this comment.
Fix browser timer typing: avoid NodeJS.Timeout in the client
Using NodeJS.Timeout in frontend code can break type-checking (DOM vs Node). Use the cross‑env safe type.
Apply this diff:
- const timeouts: NodeJS.Timeout[] = [];
+ const timeouts: ReturnType<typeof setTimeout>[] = [];
@@
- const addTimeout = (callback: () => void, delay: number) => {
- const id = setTimeout(callback, delay);
- timeouts.push(id);
- };
+ const addTimeout = (callback: () => void, delay: number) => {
+ const id = window.setTimeout(callback, delay);
+ timeouts.push(id);
+ };
@@
- return () => {
- timeouts.forEach(clearTimeout);
- };
+ return () => {
+ timeouts.forEach((id) => window.clearTimeout(id));
+ };Also applies to: 86-103
🤖 Prompt for AI Agents
In frontend/app/onboarding/onboarding-layout.tsx around lines 79-84 (and also
86-103), replace the NodeJS-specific timer typing with a cross-environment safe
type: change the timeouts array type to ReturnType<typeof setTimeout>[] (or
number[] if you prefer explicit DOM-only typing), type the id returned from
setTimeout accordingly, and ensure clearTimeout is called with that typed id;
update any related function signatures/usages (addTimeout, cleanup loop) to use
the new timer type so the code compiles under DOM types.
| } | ||
| } | ||
| tab, err := createTabObj(ctx, workspaceId, tabName, pinned || isInitialLaunch, meta) | ||
| tab, err := createTabObj(ctx, workspaceId, tabName, pinned || isInitialLaunch, nil) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that removing initial launch meta doesn't break downstream functionality.
The code now passes nil for meta instead of previously setting "waveai:panelopen": true for initial launches. Ensure that no downstream code depends on this meta being set during the initial launch flow, as its absence could affect the onboarding experience.
Run the following script to check for references to the waveai:panelopen meta key:
🏁 Script executed:
#!/bin/bash
# Description: Search for references to the waveai:panelopen meta key
# Search for the meta key in the codebase
rg -nP --type=go -C3 'waveai:panelopen|waveai.*panelopen'Length of output: 1146
Restore initial-launch panel-open meta and update modal reference
- pkg/wcore/workspace.go line 211: replace the
nilmeta arg withmap[string]interface{}{waveobj.MetaKey_WaveAiPanelOpen: true}to reinstate the AI panel opening on first launch. - pkg/wcore/workspace.go line 222: update the comment to reference “OnboardingModal” instead of “TOS modal.”
🤖 Prompt for AI Agents
In pkg/wcore/workspace.go around line 211 replace the nil meta argument to
createTabObj with a map[string]interface{} containing
waveobj.MetaKey_WaveAiPanelOpen: true so the AI panel opens on initial launch
(i.e., pass map[string]interface{}{waveobj.MetaKey_WaveAiPanelOpen: true}
instead of nil); also update the comment on line 222 to reference
“OnboardingModal” rather than “TOS modal.”
No description provided.